BUG: Properly handle lists for .mask #21934
Conversation
gfyoung
added
Bug
Algos
Codecov Report
@@ Coverage Diff @@
## master #21934 +/- ##
==========================================
- Coverage 91.96% 91.96% -0.01%
==========================================
Files 166 166
Lines 50329 50325 -4
==========================================
- Hits 46287 46283 -4
Misses 4042 4042
Continue to review full report at Codecov.
|
| @@ -7941,6 +7941,10 @@ def mask(self, cond, other=np.nan, inplace=False, axis=None, level=None, | |||
| inplace = validate_bool_kwarg(inplace, 'inplace') | |||
| cond = com._apply_if_callable(cond, self) | |||
|
|
|||
| # see gh-21891 | |||
There was a problem hiding this comment.
I would do this in .where itself
There was a problem hiding this comment.
Do what exactly in .where ?
There was a problem hiding this comment.
oh i c we are using ~ here, ok then.
pandas/tests/frame/test_indexing.py
Outdated
| @@ -3010,6 +3010,14 @@ def test_mask_callable(self): | |||
| tm.assert_frame_equal(result, | |||
| (df + 2).mask((df + 2) > 8, (df + 2) + 10)) | |||
|
|
|||
| def test_mask_list(self): | |||
There was a problem hiding this comment.
can you pas a bunch of list-likes in a fixture (the old box argument)
There was a problem hiding this comment.
Old box argument? Sorry, I'm not sure what you mean here (or I forgot what it meant if you have mentioned it to me before).
| @@ -582,6 +582,15 @@ def test_where_dt_tz_values(tz_naive_fixture): | |||
| assert_series_equal(exp, result) | |||
|
|
|||
|
|
|||
| def test_where_list(): | |||
There was a problem hiding this comment.
can you test where as well with array-likes for the cond
There was a problem hiding this comment.
can you test where as well with array-likes for the cond
That's tested pretty heavily for DataFrame and Series already in fact in the test files that I have modified in this PR.
There was a problem hiding this comment.
ok can u integrate this test in a related one then?
There was a problem hiding this comment.
I thought about that, but I was more comfortable leaving it as a separate test semantically speaking. After all, it is more atomic that way IMO.
There was a problem hiding this comment.
right but it is not clear not where things are tested. we are apparently testing list-likes (ex list) somewhere else, so this is confusing to a new reader and is just future technical debt.
There was a problem hiding this comment.
Fair enough. Integrated.
gfyoung
force-pushed
the
mask-invert-cond
branch
from
b2823a0 to
f3b3755
Compare
| @@ -7941,6 +7941,10 @@ def mask(self, cond, other=np.nan, inplace=False, axis=None, level=None, | |||
| inplace = validate_bool_kwarg(inplace, 'inplace') | |||
| cond = com._apply_if_callable(cond, self) | |||
|
|
|||
| # see gh-21891 | |||
There was a problem hiding this comment.
cond could be a callable , this patch seems wouldn't work.
There was a problem hiding this comment.
Yes, it would. Because com._apply_if_callable (right above) returns an ndarray. Passing in callable for cond is covered (and passing) in the tests.
|
thanks @gfyoung |
Title is self-explanatory.
Closes #21891.